Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a way to skip generating some sql type definitions #4002

Merged
merged 16 commits into from
May 3, 2024

Conversation

Sagebati
Copy link
Contributor

@Sagebati Sagebati commented Apr 24, 2024

I run across a problem using some third party crates. with generate_missing_sql_type_definitions

So I added a skip_missing_sql_type_definitions, it takes a vec of regexes if any matches we ignore the type.

I hope this is valuable.

@weiznich weiznich requested a review from a team April 24, 2024 17:08
Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting this PR 👍

I'm generally open to such a change but I would expect that it includes at least a few tests + passes the CI. Also we likely want to have a changelog entry for this addition as it might be useful for others as well?

@Ten0
Copy link
Member

Ten0 commented Apr 25, 2024

Wouldn't updating PgVector be a simpler option that would avoid:

  • sql type declarations in PgVector
  • additional non-trivial options in Diesel

?

@weiznich
Copy link
Member

@Ten0 I fear it's not possible for crates like pgvetcor to do that in a helpful way. They need to define their own SQL type for:

  • Implementing the necessary (de)serialization traits out of the box
  • Provide custom DSL methods for expressions of that SQL type

I do not see how you could one or the other with arbitrary types provided by a downstream crate (from point of view of pgvector) without running into limitations due to the orphan rule.

For reference their diesel implementation. I would assume that this also affects other crates like postgis-diesel

@Sagebati
Copy link
Contributor Author

Sagebati commented Apr 25, 2024

Thanks, I added a test and a CHANGELOG entry. but I struggle with the test. It feels like the diesel.toml in the test folder isn't read.

PS: I also renamed the command to except_custom_type_definitions to match the table filters.

@weiznich
Copy link
Member

Ahh, right. I always forget this. The tests do 2 things:

  • Test whether the expected code is generated via the option set in your diesel.toml. This part likely works fine
  • Test whether the expected code is generated via the option set in the CLI interface. This part is likely missing and causes the test to fail

Sorry for not mentioning that earlier.

CHANGELOG.md Outdated
@@ -12,7 +12,7 @@ Increasing the minimal supported Rust version will always be coupled at least wi

### Added

* Support `[print_schema] skip_missing_sql_type_definitions=["Vector"]`. If a `missing type` matches one element on the list it's skipped.
* Support `[print_schema] exclude_custom_sql_type_definitions=["Vector"]`. If a `missing type` matches one element on the list it's skipped.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You name it except_custom_sql_type_definitions

Suggested change
* Support `[print_schema] exclude_custom_sql_type_definitions=["Vector"]`. If a `missing type` matches one element on the list it's skipped.
* Support `[print_schema] except_custom_sql_type_definitions=["Vector"]`. If a `missing type` matches one element on the list it's skipped.

@@ -10,6 +10,14 @@ fn run_infer_schema_without_docs() {
test_print_schema("print_schema_simple_without_docs", vec![]);
}

#[test]
fn run_except_custom_type_definitions() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is run for all backends but it only contains files for postgresql. That's the reason it is failing for SQLite. There are two options to fix this:

  • Add the relevant test files for the other backends (SQLite, MySQL)
  • add a #[cfg(feature = "postgres")] to the test function to mark this test as postgres only

Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine for me now beside the unrelated changes in the Changelog.md file.

I will merge this likely next week to give @Ten0 the possibility to response to my answer above.

CHANGELOG.md Outdated
* Added automatic usage of all sqlite `rowid` aliases when no explicit primary key is defined for `print-schema`
* Added a `#[dsl::auto_type]` attribute macro, allowing to infer type of query fragment functions
* Added the same type inference on `Selectable` derives, which allows skipping specifying `select_expression_type` most of the time, in turn enabling most queries to be written using just a `Selectable` derive.
* Added an optional `#[diesel(skip_insertion)]` field attribute to the `Insertable` derive macro, allowing fields which map to generated columns to be skipped during insertion.
* Added the same type inference on `Selectable` derives, which allows skipping specifying `select_expression_type` most
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather like to not have all these unrelated changes in this file.

@Ten0
Copy link
Member

Ten0 commented Apr 26, 2024

to give @Ten0 the possibility to response to my answer above

Makes sense, didn't realize the types were so non-specific so yeah the resulting types are significantly more correct this way 👍😊

@Sagebati
Copy link
Contributor Author

Sagebati commented Apr 30, 2024

Thanks for your changes, any clue why the CI is failing again ?

@weiznich
Copy link
Member

The CI errors are caused by rust-lang/rust#124396. They are not related to this PR. I will fix them as soon as I'm back from holiday.

@weiznich weiznich added this pull request to the merge queue May 3, 2024
Merged via the queue into diesel-rs:master with commit 3306129 May 3, 2024
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants